Skip to content

{Core} Honor expiresOn, instead of expiresIn #15901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jiasli merged 5 commits into from
Dec 7, 2020
Merged

{Core} Honor expiresOn, instead of expiresIn #15901

jiasli merged 5 commits into from
Dec 7, 2020

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Nov 13, 2020 •
edited
Loading

Description

Refine #15875, #15880

#12952 uses expiresIn to calculate the expiration date:

https://github.com/Azure/azure-cli/pull/12952/files#diff-7f489fcc6ae732563b52a550ccc3c32411ec118393eafd449ee9f5ab211ebf48R64

return AccessToken(token, int(full_token['expiresIn'] + time.time()))

As tokens are cached, expiresIn saved in ~/.azure/accessTokens.json will always be a fixed value, a little bit smaller or equal to 3600. Using full_token['expiresIn'] + time.time() will result in an expiration time already past the actual expiration time (expiresOn).

Get token from server        3600s             Expiration
         |------------------------------------------|
               |------------------------------------------|
      Get token from cache                       Computed expiration

This way of computing expiration date is no different than "carving a mark on the boat to seek the sunk sword" (刻舟求剑).

Changes

  1. Honor expiresOn, instead of expiresIn.
  2. Add some token samples, to avoid future overlooked issue caused by different name of keys in the token entry ([Core] Fix get_token() issue in msi login and expiresIn key error in cloud shell login credentials for track 2 SDK related commands #14187) - User and service principal token entries have expiresOn in local time, while Managed Identity token entries has expires_on in epoch time.

Testing Guide

Test on a local machine:

az login
az extension add -n account
account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590

Test on a VM with Managed Identity:

az login --identity
az extension add -n account
account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590

To test in Cloud Shell, see #13567.

@jiasli jiasli requested review from Juliehzl, bim-msft, houk-ms, jsntcy, lmazuel and qwordy and removed request for Juliehzl November 13, 2020 06:41
@yonzhan
Copy link
Collaborator

yonzhan commented Nov 13, 2020

Core

@yonzhan yonzhan requested a review from evelyn-ys November 13, 2020 17:29
@yonzhan yonzhan added this to the S179 milestone Nov 13, 2020
@yonzhan yonzhan requested a review from arrownj November 13, 2020 17:30
@qwordy
Copy link
Member

qwordy commented Nov 16, 2020

Has Bin's PR fixed it?
Can we remove expiresIn field in token file.
When will we support file write protection of token file?

@jiasli
Copy link
Member Author

jiasli commented Nov 16, 2020

Has Bin's PR fixed it?

Those PRs still leave some unreachable code which should be removed:

return AccessToken(token, int(full_token['expiresIn'] + time.time()))

Can we remove expiresIn field in token file.

They are from ADAL cache, we prefer not to do special handling to it.

When will we support file write protection of token file?

After we migrate to MSAL.

@jiasli jiasli marked this pull request as ready for review November 18, 2020 05:12
jiasli
jiasli commented Nov 18, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/adal_authentication.py
Comment on lines +224 to +230
def _timestamp(dt):
# datetime.datetime can't be patched:
# TypeError: can't set attributes of built-in/extension type 'datetime.datetime'
# So we wrap datetime.datetime.timestamp with this function.
# https://docs.python.org/3/library/unittest.mock-examples.html#partial-mocking
# https://williambert.online/2011/07/how-to-unit-testing-in-django-with-mocking-and-patching/
return dt.timestamp()
Copy link
Member Author

@jiasli jiasli Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function serves the same purpose as azure.cli.command_modules.profile.custom._fromtimestamp introduced by #15131.

@jiasli jiasli added the Track2 label Nov 19, 2020
@jiasli
Merge branch 'dev' into expires-on
0888528 
# Conflicts:
#	src/azure-cli-core/azure/cli/core/adal_authentication.py
qwordy
qwordy reviewed Nov 19, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/tests/test_adal_authentication.py
Comment on lines +9 to +10
import unittest.mock as mock
from unittest.mock import MagicMock
Copy link
Member

@qwordy qwordy Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be improved.

Copy link
Member Author

@jiasli jiasli Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How?

Copy link
Member

@qwordy qwordy Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete 2nd line or delete 1st line and import all used class in mock

bim-msft
bim-msft approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@bim-msft bim-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Juliehzl
Juliehzl reviewed Nov 20, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/adal_authentication.py
return AccessToken(token, int(full_token['expires_on']))

from azure.cli.core.azclierror import CLIInternalError
raise CLIInternalError("No expiresOn or expires_on is available in the token entry.")
Copy link
Contributor

@Juliehzl Juliehzl Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no expected property in the token entry, should it be AAD or ADAL issue?

Copy link
Member Author

@jiasli jiasli Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I tested, it is never possible for expiresOn or expires_on to be missing.

@jiasli
Copy link
Member Author

jiasli commented Nov 30, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Nov 30, 2020

Azure Pipelines successfully started running 2 pipeline(s).

@jsntcy
Copy link
Member

jsntcy commented Nov 30, 2020

@jiasli , please fix CI failure

@jiasli
Copy link
Member Author

jiasli commented Nov 30, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Nov 30, 2020

Azure Pipelines successfully started running 2 pipeline(s).

@yonzhan yonzhan modified the milestones: S179, S180 Dec 5, 2020
@jiasli jiasli merged commit efdbcd9 into Azure:dev Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Juliehzl Juliehzl Juliehzl left review comments

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@jsntcy jsntcy jsntcy approved these changes

@lmazuel lmazuel Awaiting requested review from lmazuel

@houk-ms houk-ms Awaiting requested review from houk-ms

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

+3 more reviewers

@qwordy qwordy qwordy left review comments

@arrownj arrownj arrownj approved these changes

@bim-msft bim-msft bim-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiasli jiasli

Labels

Track2

Projects

None yet

Milestone

S180

Development

Successfully merging this pull request may close these issues.

8 participants

@jiasli @yonzhan @qwordy @jsntcy @arrownj @bim-msft @evelyn-ys @Juliehzl